Verified the premise against the Geronimo 4.0.0 sources — unassociate() and begin()
are the only places threadTx and transactionTimeoutMilliseconds get cleared, so a web
request really can strand both on a pooled exec thread. Rolling back at request teardown
is the right fix and matches TxBeanManaged.commit(). The single-exec-thread test is a
genuinely good reproduction, and the guard that fails loudly if thread reuse didn't happen
is the right instinct.

The placement rationale is wrong, though, and it should be corrected in the javadoc
because it changes what applications observe:

• The description says the valve runs after ServletRequestListener#requestDestroyed.
  It's the opposite. I disassembled tomcat-catalina 11.0.23: StandardContextValve has no
  fireRequest* call at all. StandardHostValve.invoke fires fireRequestInitEvent at
  offset 72, invokes the Context pipeline (where OpenEJBValve and therefore clean()
  live) at offset 125, and fires fireRequestDestroyEvent only at offset 327 — after the
  pipeline returns.

  So an application ServletRequestListener implementing a tx-per-request pattern and
  committing in requestDestroyed now finds the transaction already rolled back, gets a
  WARN on every single request, and an IllegalStateException from its own commit().
  Filter- and servlet-based patterns are unaffected, so this doesn't invalidate the fix —
  but pre-empting requestDestroyed is a real behavioural change and belongs in the
  javadoc.

• Third uncovered path, more concrete than the async ones: StandardHostValve.invoke
  calls throwable()/status() at offsets 195/298/307, after the pipeline, and
  custom() dispatches the error page through ApplicationDispatcher.include(), not a
  Pipeline. So <error-page> servlets and JSPs run after clean() — they leak exactly as
  before this PR, and they now also run with the request's transaction already rolled back
  and the caller identity already cleared. If you want that covered,
  OpenEJBSecurityListener.RequestCapturer on the Host pipeline
  (TomcatWebAppBuilder:317) wraps all of it.

Smaller:

• In OpenEJBValve, TransactionCleanup.clean() is skipped if listener.exit() throws.
  The async path already gets this right with a nested finally — please mirror it here.
• setTransactionTimeout(0) pins the very ThreadLocal entry that the
  CoreUserTransaction.resetError hunk in this same PR argues against pinning. Worth a
  comment explaining why the tradeoff differs, or reading the timeout first and only
  resetting when non-zero.
• The timeout reset is never asserted. Leaker sets setTransactionTimeout(120) with a
  comment saying it must not leak, and then nothing checks it — the branch is untested.
• Unconditional rollback() logs an ERROR when the leftover transaction is no longer
  rollback-able (already rolled back / marked for rollback by a timeout reaper). Check
  getStatus() against STATUS_ROLLEDBACK/STATUS_ROLLING_BACK first, or log at debug.

Merge precondition rather than a code comment: please re-run the Jakarta Transactions TCK
and drop the corresponding exclusions in apache/tomee-tck with this, since that's the
harness that surfaced it.

Thanks — the ordering correction was right, and I verified it independently before acting on it (javap on tomcat-catalina 11.0.23: StandardContextValve has zero fireRequest* calls; StandardHostValve.invoke has fireRequestInitEvent at 72, the Context pipeline at 113, throwable()/status() at 195/298/307, and fireRequestDestroyEvent at 327).

Rather than only fix the javadoc, I moved the cleanup to OpenEJBSecurityListener.RequestCapturer on the Host pipeline, which wraps all of StandardHostValve#invoke. That covers requestDestroyed, the throwable()/status() paths, and <error-page> servlets/JSPs, and it stops pre-empting applications that complete their transaction in requestDestroyed. The javadoc now describes the real ordering. This also removes the OpenEJBValve call site entirely, so the "skipped if listener.exit() throws" hole goes away with it.

Also addressed:

• Already-rolled-back status: rollback() now checks STATUS_ROLLEDBACK/STATUS_ROLLING_BACK and logs at debug in that case, warning otherwise.
• Timeout reset pinning: kept unconditional, with a comment explaining why the tradeoff differs from resetError. Reading the timeout first isn't available — Geronimo exposes no getter, only the package-private getTransactionTimeoutMilliseconds(long). And setTransactionTimeout(0) stores a null value, which can't pin a webapp classloader the way a stored exception's stack trace can.
• Timeout never asserted: you were right that the branch was untested. Committer now reads the effective timeout of its own transaction and the test distinguishes the leaked 120s from the 600s default. Worth noting the first version of this assertion was a false green — the field is an absolute deadline (duration + currentTime), not a duration, so comparing it to 120000 could never match. Fixed to compare the remaining duration, and confirmed it fails when the cleanup is removed.

On the merge precondition: I have not run the Jakarta Transactions TCK, so the exclusions in apache/tomee-tck are untouched and this shouldn't merge on my testing alone. tomee-catalina and tomee-embedded suites are green.

🤖 Addressed by Claude Code

Please run a CI full build for this.

https://ci-builds.apache.org/job/Tomee/view/tomee-11.x/job/pull-request-manual-with-tck/56/